feat: Microsoft.Extensions.AI support #64
Conversation
WalkthroughThe changes in this pull request introduce enhancements to the documentation and project structure of the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
README.md (1)
15-15: Consider adding usage examples for Microsoft.Extensions.AI integration.While the feature is documented in the Features section, it would be helpful to add usage examples demonstrating how to use the Microsoft.Extensions.AI integration, similar to the existing examples for other features.
Would you like me to help generate usage examples for the Microsoft.Extensions.AI integration?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
README.md(1 hunks)src/libs/Ollama/Ollama.csproj(1 hunks)src/libs/Ollama/OllamaApiClient.IChatClient.cs(1 hunks)
🔇 Additional comments (5)
src/libs/Ollama/Ollama.csproj (2)
15-15: LGTM! Package tags accurately reflect new AI capabilities.
The added tags (ai, abstractions, llama, ichatclient) appropriately describe the new Microsoft.Extensions.AI integration.
20-20: Verify compatibility of preview package with target frameworks.
The Microsoft.Extensions.AI.Abstractions package is currently in preview (9.0.0-preview.9) and targets .NET 9. This raises several concerns:
- Using preview packages in production code could lead to breaking changes
- Need to verify compatibility with older target frameworks (netstandard2.0, net4.6.2)
Consider:
- Adding conditional package references based on target framework
- Documenting the preview status in README
- Having a plan to update when the stable version is released
✅ Verification successful
Let me gather more information about the target frameworks and package references.
Let me gather more information about the AI abstractions package usage.
Based on the analysis of the codebase, I can now provide a final response:
Preview package usage is appropriate for this implementation
The Microsoft.Extensions.AI.Abstractions package is used appropriately in this context because:
- The project properly implements the IChatClient interface from the package
- The project supports multiple target frameworks (netstandard2.0, net4.6.2, net8.0) as defined in the csproj
- The package is only used for interface implementation, which is framework-agnostic
- The preview status is already documented in the README.md
The preview package doesn't pose compatibility risks since:
- The implementation is focused on interface contracts rather than framework-specific features
- The project uses conditional compilation (#if NET8_0_OR_GREATER) to handle framework-specific code
- PolySharp is included to handle polyfills for older frameworks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any documented known issues or breaking changes
rg -A 5 "Microsoft\.Extensions\.AI" README.md CHANGELOG.md docs/
# Look for any conditional compilation or framework checks related to this feature
ast-grep --pattern 'if NETSTANDARD || NET462 {
$$$
}'
Length of output: 389
Script:
#!/bin/bash
# Check target frameworks in the project file
cat src/libs/Ollama/Ollama.csproj
# Check for any conditional package references or framework-specific code
rg -g "*.cs" -B 2 -A 2 "#if" src/libs/Ollama/
# Look for any framework-specific implementations
ast-grep --pattern 'namespace $_ {
$$$
#if $_
$$$
#endif
$$$
}'
Length of output: 79561
Script:
#!/bin/bash
# Check for AI abstractions usage in the codebase
rg -g "*.cs" -B 2 -A 2 "Microsoft\.Extensions\.AI" src/
# Check for any framework-specific implementations or interfaces
ast-grep --pattern 'interface $_ : Microsoft.Extensions.AI.$_ {
$$$
}'
Length of output: 612
README.md (1)
15-15: LGTM! Feature documentation added.
The addition of Microsoft.Extensions.AI support in the Features section is clear and includes a link to the official documentation.
src/libs/Ollama/OllamaApiClient.IChatClient.cs (2)
34-34: Verify handling of null Temperature in RequestOptions
When options is null or options.Temperature is not set, Temperature will be null. Ensure that the RequestOptions object and the downstream processing can handle a null Temperature appropriately, or provide a default value to maintain expected behavior.
29-31: Confirm default behavior for ResponseFormat when null
If options?.ResponseFormat does not match ChatResponseFormatJson, the format parameter is set to null. Verify that passing a null format to GenerateChatCompletionAsync is acceptable and that it triggers the intended default behavior in the API.
| ChatOptions? options = null, | ||
| CancellationToken cancellationToken = default) | ||
| { | ||
| throw new NotImplementedException(); |
There was a problem hiding this comment.
Implement CompleteStreamingAsync method
The CompleteStreamingAsync method currently throws a NotImplementedException. Since this method is part of the IChatClient interface, it should be properly implemented to fulfill the contract and avoid runtime exceptions.
Would you like assistance in implementing the CompleteStreamingAsync method?
| Parameters = x.AsJson(), | ||
| }, | ||
| }).ToList(), | ||
| cancellationToken: cancellationToken).WaitAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
Review the use of WaitAsync with ConfigureAwait(false)
Using WaitAsync() in combination with await and ConfigureAwait(false) may be unnecessary and could introduce complexity. Consider removing WaitAsync() to simplify the code unless there is a specific need for it.
Apply this diff to streamline the await statement:
- cancellationToken: cancellationToken).WaitAsync().ConfigureAwait(false);
+ cancellationToken: cancellationToken).ConfigureAwait(false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cancellationToken: cancellationToken).WaitAsync().ConfigureAwait(false); | |
| cancellationToken: cancellationToken).ConfigureAwait(false); |
| Role = x.Role.Value switch | ||
| { | ||
| "assistant" => MessageRole.Assistant, | ||
| "user" => MessageRole.User, | ||
| "system" => MessageRole.System, | ||
| "tool" => MessageRole.Tool, | ||
| _ => MessageRole.User, | ||
| }, |
There was a problem hiding this comment.
Handle potential null values in x.Role
The code uses x.Role.Value without verifying if x.Role or x.Role.Value is null. This could lead to a NullReferenceException if x.Role is null. To prevent this, consider adding a null-conditional operator or a null check.
Apply this diff to safely access x.Role.Value:
- Role = x.Role.Value switch
+ Role = (x.Role?.Value) switch📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Role = x.Role.Value switch | |
| { | |
| "assistant" => MessageRole.Assistant, | |
| "user" => MessageRole.User, | |
| "system" => MessageRole.System, | |
| "tool" => MessageRole.Tool, | |
| _ => MessageRole.User, | |
| }, | |
| Role = (x.Role?.Value) switch | |
| { | |
| "assistant" => MessageRole.Assistant, | |
| "user" => MessageRole.User, | |
| "system" => MessageRole.System, | |
| "tool" => MessageRole.Tool, | |
| _ => MessageRole.User, | |
| }, |
| tools: options?.Tools?.Select(x => new Tool | ||
| { | ||
| Function = new ToolFunction | ||
| { | ||
| Name = string.Empty, | ||
| Description = string.Empty, | ||
| Parameters = x.AsJson(), | ||
| }, | ||
| }).ToList(), |
There was a problem hiding this comment.
Provide meaningful Name and Description for tools
Currently, the Name and Description properties of ToolFunction are set to string.Empty. This may not provide sufficient information for tool functionality. Consider populating these properties with relevant values from x to enhance clarity and utility.
Apply this diff to assign appropriate values:
Function = new ToolFunction
{
- Name = string.Empty,
- Description = string.Empty,
+ Name = x.Name,
+ Description = x.Description,
Parameters = x.AsJson(),
},Committable suggestion skipped: line range outside the PR's diff.
| var response = await Chat.GenerateChatCompletionAsync( | ||
| model: options?.ModelId ?? "ollama", | ||
| messages: chatMessages.Select(x => new Message | ||
| { | ||
| Content = x.Text ?? string.Empty, | ||
| Role = x.Role.Value switch | ||
| { | ||
| "assistant" => MessageRole.Assistant, | ||
| "user" => MessageRole.User, | ||
| "system" => MessageRole.System, | ||
| "tool" => MessageRole.Tool, | ||
| _ => MessageRole.User, | ||
| }, | ||
| }).ToArray(), | ||
| format: options?.ResponseFormat switch | ||
| { | ||
| ChatResponseFormatJson => ResponseFormat.Json, | ||
| _ => null, | ||
| }, | ||
| options: new RequestOptions | ||
| { | ||
| Temperature = options?.Temperature, | ||
| }, | ||
| stream: false, | ||
| keepAlive: default, | ||
| tools: options?.Tools?.Select(x => new Tool | ||
| { | ||
| Function = new ToolFunction | ||
| { | ||
| Name = string.Empty, | ||
| Description = string.Empty, | ||
| Parameters = x.AsJson(), | ||
| }, | ||
| }).ToList(), | ||
| cancellationToken: cancellationToken).WaitAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
Ensure response is not null before accessing its properties
Currently, the code checks if response.Message is null but does not verify if response itself is null. If Chat.GenerateChatCompletionAsync returns null, accessing response.Message will result in a NullReferenceException. Consider adding a null check for response to enhance robustness.
Apply this diff to add the null check:
+ if (response == null)
+ {
+ throw new InvalidOperationException("Response was null.");
+ }
if (response.Message == null)
{
throw new InvalidOperationException("Response message was null.");
}Committable suggestion skipped: line range outside the PR's diff.
https://github.com/dotnet/extensions/tree/main/src/Libraries/Microsoft.Extensions.AI.Abstractions
https://devblogs.microsoft.com/dotnet/introducing-microsoft-extensions-ai-preview/
awaescher/OllamaSharp#111
Summary by CodeRabbit
New Features
Microsoft.Extensions.AIlibrary in the README.OllamaApiClientclass with methods for chat interactions, includingCompleteAsyncfor chat completion.Documentation
Chores
Microsoft.Extensions.AI.Abstractions.